Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle transmission mode reference #419

Merged
merged 43 commits into from
Mar 31, 2022
Merged

Handle transmission mode reference #419

merged 43 commits into from
Mar 31, 2022

Conversation

fmigneault
Copy link
Collaborator

@fmigneault fmigneault commented Mar 17, 2022

Changes

Many adjustments for OGC compliance.

  • Everything from first implementation prefer header for execute job #415
  • Properly return the results directly when running in sync mode (instead of status info)
  • Generate the Link header for outputs requested with transmissionMode=reference
  • Some jobs utils refactors to align with new supported features

References

@fmigneault fmigneault self-assigned this Mar 17, 2022
@github-actions github-actions bot added ci/doc Issue related to documentation of the package ci/operations Related to CI operations (actions, execution, install, builds, etc.) ci/tests Tests of the package and features feature/docker Issue related to Docker application package execution. process/builtin Issue related to builtin application processes labels Mar 17, 2022
@github-actions github-actions bot added feature/cli Issues or features related to CLI operations. feature/db Related to database or datatype manipulation. feature/job Issues related to job execution, reporting and logging. feature/oas Issues related to OpenAPI specifications. feature/opensearch Issue related to OpenSearch functionalities. feature/providers Issue related to providers convertion to WPS-REST processes. labels Mar 17, 2022
@fmigneault fmigneault added the project/OGC Related to OGC testbeds or relavant projects. label Mar 23, 2022
@fmigneault fmigneault marked this pull request as ready for review March 24, 2022 00:27
@fmigneault fmigneault requested a review from dbyrns March 24, 2022 00:27
if job.status in JOB_STATUS_CATEGORIES[StatusCategory.RUNNING]:
# signal to stop celery task. Up to it to terminate remote if any.
LOGGER.debug("Job [%s] dismiss operation: Canceling task [%s]", job.id, job.task_id)
celery_app.control.revoke(job.task_id, terminate=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the revoke command is asynchronous, could the cleanup following it (removing logs, xml, dir) be done prematurily while the process is still spitting logs waiting to be killed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a possibility. I haven't explored this much.
If we encounter this, we could think of a way to handle it. Probably need to run another task with a delay to do a cleanup with below checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the succeeded jobs also leave their stuff, so a mechanism to clean that should already be in place anyway. For the typical birds dumping the result in the output directory, there was a cron job for that. Does it apply to Weaver also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results that were generated by pywps and cwltool should be all cleaned up following execution by weaver after it moved them to wps-outputs. There should be 3 items {jobId}.log, {jobId}.xml (WPS status), {jobId}/ (output results).

The dismiss cleanup applied here is more specific for OGC Dismiss operation that requests explicitly a cleanup of those outputs, given a user downloaded them.

If the cron job runs inside wps-outputs/ for each birds, then it should pick up weaver as well, but there is not a cron specifically within weaver.

Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goog PR. There is still an open comment, but it's not a deal breaker.

@fmigneault fmigneault merged commit 9ca6bbe into master Mar 31, 2022
@fmigneault fmigneault deleted the output-ref branch March 31, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/doc Issue related to documentation of the package ci/operations Related to CI operations (actions, execution, install, builds, etc.) ci/tests Tests of the package and features feature/cli Issues or features related to CLI operations. feature/db Related to database or datatype manipulation. feature/docker Issue related to Docker application package execution. feature/job Issues related to job execution, reporting and logging. feature/oas Issues related to OpenAPI specifications. feature/opensearch Issue related to OpenSearch functionalities. feature/providers Issue related to providers convertion to WPS-REST processes. process/builtin Issue related to builtin application processes process/wps3 Issue related to WPS 3.x (REST-JSON) processes support project/OGC Related to OGC testbeds or relavant projects.
Projects
None yet
2 participants